GradleFiles project scan should adhere to global root settings#9263
GradleFiles project scan should adhere to global root settings#9263sid-srini wants to merge 3 commits into
Conversation
mbien
added
Gradle
|
Hi @lkishalmi. Please review this when you get a chance. Thank you. |
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
In general makes sense to me. I left a few inline comments.
| for (String scanRoot : projectScanRoots) { | ||
| if (path.startsWith(scanRoot) | ||
| && (path.length() == scanRoot.length() | ||
| || path.charAt(scanRoot.length()) == '/')) |
There was a problem hiding this comment.
I assume this change intents to check that we don't get a substring match for the last component. So far so sensible, but will it work on windows paths?
PS: Also does not match logic in GradleFiles.Searcher#notWithinProjectScanRoots
There was a problem hiding this comment.
Thanks @matthiasblaesing for your review and the close attention on these path comparison differences.
As noted in this class, in the separatePaths method, projectScanRoots store FileObject.getPath() values since getOwner supplies and compares with FileObject.getPath() strings. The FileObject instances canonicalize and always return / as the file-separator character, even on Windows.
This explicitly does not match the logic in GradleFiles.Searcher#notWithinProjectScanRoots which compares with File.separatorChar, because there GradleFiles uses File instance path comparisons instead. Thus, GradleFiles.Searcher#separatePaths notes that it needs to store both absolute and canonical paths in projectScanRoots.
I hope this clarifies the difference in approach.
|
Thank you @matthiasblaesing for your review and feedback. |
Fixed GradleFiles scan for project files to honour the global scan root settings, if any. - Similar to that in projectapi SimpleFileOwnerQueryImpl. - GradleFiles needs to necessarily traverse the parent directory hierarchy to find the root project settings. - Further, the project detection needs to give precedence to maven projects also defined for the same directory, which has a similar hierarchy traversal requirement introduced in apache#1280. Fixed GradleFiles to find parentScript for kotlin script. - Added a unit test case for it in GradleFilesTest. Fixed SimpleFileOwnerQueryImplementation to check for scan root dirs without allowing sibling dirs with the same name prefix. - Example: if scan root = "/tmp/app", - then "/tmp/application" should not be allowed for scan; - while "/tmp/app" and "/tmp/app/src" should remain allowed. Fixed GradleFilesTest for File comparison on MacOS. - Needed due to the presence of "/private" prefix in the canonical path of unix temp folders. Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
Split the class into 2 classes:
- GradleFilesWithScanRootTest now only runs the GradleFilesTest
with the project limitScanRoot and forbiddenFolders
properties set.
- GradleFilesScanRootTest contains only the functionality of the
scan-root and forbidden folders specifically.
- Only a single JUnit @test can be used here since the static
initialization in @before must be done once only.
- This means the test root folder must not change between tests.
- The actual gradle project is now a grandchild in the tree of the
test root dir.
- The original failure was due to the alternative placement of the
tests temporary folder on linux which is not sufficiently nested
within the OS tmp directory.
Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
1. Fixed the separatePaths() method in GradleFiles.Searcher and SimpleFileOwnerQueryImplementation to not trim() the split paths and only ignore completely blank paths. 2. Added implementation notes linking the logic and code of the above two classes so that changes made in the future are done to both. Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
sid-srini
force-pushed
the
gradle-files-scan-root
branch
from
af01fc0 to
53c3535
Compare
|
I've rebased this to the current master which includes post NB30 changes. |
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
Looks sane to me. Thank you.
|
@sid-srini would you please squash the commits? |
3 participants
Fixed
GradleFilesscan for project files to honour the global scan root settings, if any.SimpleFileOwnerQueryImpl.GradleFilesneeds to necessarily traverse the parent directory hierarchy to find the root project settings.Fixed
GradleFilesto findparentScriptfor kotlin script.GradleFilesTest.Fixed
SimpleFileOwnerQueryImplementationto check for scan root dirs without allowing sibling dirs with the same name prefix.Fixed
GradleFilesTestfor File comparison on MacOS.^Add meaningful description above
Click to collapse/expand PR instructions
By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
Please make sure (eg.
git log) that all commits have a valid name and email address for you in the Author field.If you're a first time contributor, see the Contributing guidelines for more information.
If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.
PR approval and merge checklist:
If this PR targets the delivery branch: don't merge. (full wiki article)